Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SQL Server: if possible, use OFFSET and FETCH instead of JDBC methods #300

Merged
merged 1 commit into from
Dec 14, 2018

Conversation

rdicroce
Copy link
Contributor

@rdicroce rdicroce commented Dec 3, 2018

Currently, EclipseLink uses Statement#setMaxRows() to apply a result limit when the target database is MS SQL Server. This is dangerous because mssql-jdbc implements setMaxRows() by executing SET ROWCOUNT, which applies to all queries made on the connection until SET ROWCOUNT is executed again.

Normally, that is not an issue, but it can become a problem if, for example, your query includes a call to a user-defined function that executes another query. When that happens, it creates truly baffling bugs that are very difficult to track down. mssql-jdbc acknowledges that SET ROWCOUNT is not an appropriate way to implement setMaxRows() but currently has no ability to replace it with something better; see microsoft/mssql-jdbc#176.

This PR alters SQLServerPlatform to add the OFFSET and FETCH NEXT clauses to the query, if the version of SQL Server in use supports them. This avoids EclipseLink calling setMaxRows(), and may improve query performance.

This PR is for 2.7. If it looks okay to whoever reviews it, I will open another PR for master.

Signed-off-by: Richard DiCroce <Rich.DiCroce@scientificgames.com>
Copy link
Member

@lukasj lukasj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@lukasj lukasj merged commit 0ce1cbc into eclipse-ee4j:2.7 Dec 14, 2018
@lukasj
Copy link
Member

lukasj commented Dec 14, 2018

create a PR for master, please. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants